Skip to content

Conversation

@DiamondJoseph
Copy link
Contributor

@DiamondJoseph DiamondJoseph commented Jul 24, 2025

Prep work for #689

Checklist

  • Add a Changelog entry
  • Add the ticket number which this PR closes to the comment section

@DiamondJoseph DiamondJoseph mentioned this pull request Jul 29, 2025
2 tasks
@DiamondJoseph DiamondJoseph marked this pull request as ready for review July 30, 2025 13:15
Copy link
Member

@danielballan danielballan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

I left a suggestion, a question, and an opinion for you to consider by freely pass over as you will. :-)

class Structure(ABC):
@classmethod
# TODO: When dropping support for Python 3.10 replace with -> Self
def from_json(cls, structure: Mapping[str, Any]) -> "Structure":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this, but I would prefer not to.

When I see:

class COOStructure:
   ...

I know that everything I need to read is in front of me. When I see:

class COOStructure(Structure):
    ...

I have to cross-reference. The cross-referencing is worthwhile if we can avoid significant repetition. But given that the among of DRY-ness that this achieves is quite small, I would be happier repeating this out in the specific classes.

However, YMMV, and if this seems much better to Team DLS, I do not object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After #1036 I want to change e.g.

STRUCTURE_TYPES = OneShotCachedMap(...)
# to
STRUCTURE_TYPES = OneShotCachedMap[StructureFamily, type[Structure]](...)

When we then access the types inside, I want typing and my IDE to help me out with

            structure_type = STRUCTURE_TYPES[attributes["structure_family"]]
            self._structure = structure_type.from_json(attributes["structure"])

Structure could become a Protocol here, since it's just a contract that the Map will only contain types that can be deserialised.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From trying this out, it seems Pydantic is not happy if you try and use protocols with a generic BaseModel with bounds, so I've had to revert to an ABC.

class Proto(Protocol):
    ...

P = TypeVar("P", bound=Proto)

class DataSource(BaseModel, Generic[P]):
    t: Optional[P]


def project_form(form, form_keys_touched):
def project_form(
form: awkward.forms.Form, form_keys_touched
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
form: awkward.forms.Form, form_keys_touched
form: awkward.forms.Form, form_keys_touched: List[str]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        form_keys_touched = set(report.data_touched)

Looks like it's a set in the only instance it's used. I'll make it an Iterable to prevent issues.

@DiamondJoseph DiamondJoseph force-pushed the typed_structures branch 2 times, most recently from 30d6f59 to 296fefe Compare August 22, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants